Build route from hop pubkey list. #1491

tnull · 2022-05-20T10:46:06Z

This PR introduces a new function build_route_from_hops which allows to build a route just consisting of the given hops.

It re-uses the logic from find_route/get_route, but limits the graph discovery through usage of a custom scorer.

lightning/src/routing/scoring.rs

codecov-commenter · 2022-05-20T11:03:43Z

Codecov Report

Merging #1491 (ba48ac8) into main (aac3907) will increase coverage by 0.07%.
The diff coverage is 85.00%.

❗ Current head ba48ac8 differs from pull request most recent head 2010670. Consider uploading reports for the commit 2010670 to get more accurate results

@@            Coverage Diff             @@
##             main    #1491      +/-   ##
==========================================
+ Coverage   90.95%   91.02%   +0.07%     
==========================================
  Files          80       80              
  Lines       42724    43373     +649     
  Branches    42724    43373     +649     
==========================================
+ Hits        38858    39482     +624     
- Misses       3866     3891      +25

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`92.39% <85.00%> (-0.07%)`	⬇️
lightning/src/ln/monitor_tests.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/functional_tests.rs	`97.16% <0.00%> (+0.04%)`	⬆️
lightning/src/chain/channelmonitor.rs	`91.94% <0.00%> (+0.87%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aac3907...2010670. Read the comment docs.

tnull · 2022-05-20T12:26:02Z

Rebased on #1490 to fix failing tests.

TheBlueMatt · 2022-05-20T18:23:09Z

Oops, lol, looks like it needs rebase already :(

lightning/src/routing/scoring.rs

TheBlueMatt · 2022-05-20T18:34:22Z

lightning/src/routing/router.rs

+/// Re-uses logic from `find_route`, so the restrictions described there also apply here.
+pub fn build_route_from_hops<L: Deref>(
+	our_node_pubkey: &PublicKey, hops: &[PublicKey], route_params: &RouteParameters, network: &NetworkGraph,
+	logger: L, random_seed_bytes: &[u8; 32]) -> Result<Route, LightningError>


Lets skip the random seed? Its not like it is adding any randomization in this case - we can just use [0; 32], no?

Well technically I assume we could create MPP routes over different permutations of channels for the given hops? And in this having the randomization can't hurt?

Also, while I generally agree that requiring a [u8; 32] of random bytes is not the most intuitive API which could be improved, I don't see why we shouldn't keep the signature of build_route_from_hops not more or less consistent with the one of find_route?

Well technically I assume we could create MPP routes over different permutations of channels for the given hops? And in this having the randomization can't hurt?

I...guess? I mean in practice nodes (except for LDK, sadly) ignore the SCID you provide it and just route over any available channel to the same peer, so it won't change behavior, only the route itself.

Also, while I generally agree that requiring a [u8; 32] of random bytes is not the most intuitive API which could be improved,

I think its probably the most practical, if not the most intuitive. Given the downstream issues we've seen with LDK -> user trait implementation reentrancy I want to lean even harder on the "avoid reentrancy to user code if at all possible" design goal going forward.

I don't see why we shouldn't keep the signature of build_route_from_hops not more or less consistent with the one of find_route?

Sure, it just seems like an extraneous parameter, if you strongly prefer it, so be it.

TheBlueMatt · 2022-05-20T18:35:07Z

lightning/src/routing/router.rs

+	}
+}
+
+fn build_route_from_hops_internal<L: Deref>(


I don't quite understand why this is split into two functions?

Well, this is kind of a remnant of my prior approach where I would implement much more functionality from scratch and therefore would add much more test cases for this implementation, for example in limits_total_cltv_delta. This doesn't make too much sense now because get_route is already covered, but I still kind of like the separation of the 'build route' and 'add shadow route' steps this allows. That said, I can build_route_from_hops and build_route_from_hops_internal, if you prefer.

Sure, no super strong opinion it just looked weird. Happy to leave it.

I think it avoids a bunch of what would otherwise be confusing nesting (hint: there are places that could benefit from some method splitting)

lightning/src/routing/router.rs

tnull · 2022-05-21T12:15:45Z

Oops, lol, looks like it needs rebase already :(

Rebased on main.

tnull · 2022-05-24T19:18:46Z

Rebased again to fix tests.

TheBlueMatt · 2022-05-24T20:18:19Z

This basically LGTM, do you mind squashing down the fixup commits?

tnull · 2022-05-24T20:29:18Z

This basically LGTM, do you mind squashing down the fixup commits?

Done.

lightning/src/routing/router.rs

lightning/src/routing/scoring.rs

arik-so · 2022-05-25T21:06:48Z

lightning/src/routing/router.rs

@@ -5486,6 +5529,26 @@ mod tests {
 		assert!(path_plausibility.iter().all(|x| *x));
 	}

+	#[test]
+	fn builds_correct_path_from_hops() {


Perhaps add a scenario where there are multiple channels between some or multiple pairs of hops, and that a certain path combination gets found with those distinct channels?

Mh, does determinism in the path actually matter? In contrary I thought we want to move towards path randomization (cf. #495) and even using alternate channels for forwarding (cf. #1278)?

Sorry, let me clarify. The idea is not to test determinism, but rather that it is able to find multiple paths if there are multiple that match the given criteria.

I'd think the only case where we'd care about which specific subpaths are selected would be in MPP contexts, no? Like, in general I'd think the API here doesn't intend to suggest only a specific subpath would be selected?

jkczyz

Meta comment on commits: I think this can be a single commit, especially if DynamicPenaltyScorer is removed. No need to separate out tests from implementation for something small-ish.

The commit message could be expanded to give the what and the why for posterity. Here's a good set of guidelines to follow: https://cbea.ms/git-commit/.

lightning/src/routing/scoring.rs

lightning/src/routing/router.rs

tnull · 2022-05-26T00:42:15Z

lightning/src/routing/router.rs

+		fn payment_path_successful(&mut self, _path: &[&RouteHop]) {}
+	}
+
+	let scorer = HopScorer { hops: &[&[*our_node_pubkey],hops].concat() };


Not the prettiest, might have another look at this.

Don't have a strong opinion, but note that we're trading cloning some iterators previously for allocating a Vec here.

lightning/src/routing/router.rs

jkczyz · 2022-05-26T05:21:21Z

lightning/src/routing/router.rs

+		fn payment_path_successful(&mut self, _path: &[&RouteHop]) {}
+	}
+
+	let scorer = HopScorer { hops: &[&[*our_node_pubkey],hops].concat() };


Don't have a strong opinion, but note that we're trading cloning some iterators previously for allocating a Vec here.

lightning/src/routing/router.rs

jkczyz

For commit messages, as per previously cited reference, please:

don't end summary line with a period
wrap body lines at 72 characters

lightning/src/routing/router.rs

Implements `build_route_from_hops`, which provides a simple way to build a route from us (payer) to the target node (payee) via the given hops (which should exclude the payer, but include the payee). This may be useful, e.g., for probing the chosen path.

tnull commented May 20, 2022

View reviewed changes

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

tnull force-pushed the 2022-05-build-route-from-pubkeys branch 2 times, most recently from 1842b5a to 079405b Compare May 20, 2022 12:25

TheBlueMatt reviewed May 20, 2022

View reviewed changes

tnull force-pushed the 2022-05-build-route-from-pubkeys branch 2 times, most recently from c53a6c2 to 943b951 Compare May 21, 2022 12:14

TheBlueMatt added the Seeking Code Review label May 21, 2022

tnull force-pushed the 2022-05-build-route-from-pubkeys branch from 943b951 to f4924d9 Compare May 24, 2022 19:18

tnull force-pushed the 2022-05-build-route-from-pubkeys branch from f4924d9 to f10303a Compare May 24, 2022 20:28

arik-so reviewed May 25, 2022

View reviewed changes

TheBlueMatt removed the Seeking Code Review label May 25, 2022

jkczyz reviewed May 25, 2022

View reviewed changes

tnull force-pushed the 2022-05-build-route-from-pubkeys branch 2 times, most recently from f6607a2 to 19ecb4c Compare May 26, 2022 00:36

tnull commented May 26, 2022

View reviewed changes

jkczyz reviewed May 26, 2022

View reviewed changes

TheBlueMatt added this to the 0.0.107 milestone May 26, 2022

tnull force-pushed the 2022-05-build-route-from-pubkeys branch from 19ecb4c to ba48ac8 Compare May 26, 2022 22:34

TheBlueMatt reviewed May 26, 2022

View reviewed changes

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

tnull force-pushed the 2022-05-build-route-from-pubkeys branch from ba48ac8 to c88c71e Compare May 26, 2022 23:34

TheBlueMatt previously approved these changes May 26, 2022

View reviewed changes

jkczyz reviewed May 27, 2022

View reviewed changes

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

tnull dismissed TheBlueMatt’s stale review via 2010670 May 27, 2022 00:30

tnull force-pushed the 2022-05-build-route-from-pubkeys branch from c88c71e to 2010670 Compare May 27, 2022 00:30

jkczyz approved these changes May 27, 2022

View reviewed changes

TheBlueMatt approved these changes May 27, 2022

View reviewed changes

TheBlueMatt merged commit cc374ec into lightningdevkit:main May 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build route from hop pubkey list. #1491

Build route from hop pubkey list. #1491

tnull commented May 20, 2022

codecov-commenter commented May 20, 2022 •

edited

Loading

tnull commented May 20, 2022

TheBlueMatt commented May 20, 2022

TheBlueMatt May 20, 2022

tnull May 21, 2022

TheBlueMatt May 21, 2022

TheBlueMatt May 20, 2022

tnull May 21, 2022 •

edited

Loading

TheBlueMatt May 21, 2022 •

edited

Loading

arik-so May 26, 2022

tnull commented May 21, 2022 •

edited

Loading

tnull commented May 24, 2022 •

edited

Loading

TheBlueMatt commented May 24, 2022

tnull commented May 24, 2022 •

edited

Loading

arik-so May 25, 2022

tnull May 25, 2022

arik-so May 25, 2022

TheBlueMatt May 26, 2022

jkczyz left a comment

tnull May 26, 2022

jkczyz May 26, 2022

jkczyz May 26, 2022

jkczyz left a comment

Build route from hop pubkey list. #1491

Build route from hop pubkey list. #1491

Conversation

tnull commented May 20, 2022

codecov-commenter commented May 20, 2022 • edited Loading

Codecov Report

tnull commented May 20, 2022

TheBlueMatt commented May 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull May 21, 2022 • edited Loading

Choose a reason for hiding this comment

TheBlueMatt May 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented May 21, 2022 • edited Loading

tnull commented May 24, 2022 • edited Loading

TheBlueMatt commented May 24, 2022

tnull commented May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 20, 2022 •

edited

Loading

tnull May 21, 2022 •

edited

Loading

TheBlueMatt May 21, 2022 •

edited

Loading

tnull commented May 21, 2022 •

edited

Loading

tnull commented May 24, 2022 •

edited

Loading

tnull commented May 24, 2022 •

edited

Loading